Skip to content

Add options class for table update, fix put in client#404

Merged
romanbracinik merged 1 commit intomasterfrom
roman-fix-update-table-in-client
Apr 23, 2020
Merged

Add options class for table update, fix put in client#404
romanbracinik merged 1 commit intomasterfrom
roman-fix-update-table-in-client

Conversation

@romanbracinik
Copy link
Copy Markdown
Contributor

@romanbracinik romanbracinik commented Apr 16, 2020

Screenshot 2020-04-16 at 17 03 39

@romanbracinik romanbracinik marked this pull request as ready for review April 16, 2020 15:04
Copy link
Copy Markdown
Contributor

@tomasfejfar tomasfejfar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Viděl jsem funkční testy.

image

@zajca
Copy link
Copy Markdown
Member

zajca commented Apr 22, 2020

Ale to je přece BC

@tomasfejfar
Copy link
Copy Markdown
Contributor

tomasfejfar commented Apr 22, 2020

No je to BC break v klientu. To, že to posílalo parametry v getu je bug resp. "nedokumentované náhodně funkční chování", ne kontakt. (resp. tak to beru já)

@ErikZigo
Copy link
Copy Markdown
Contributor

Tohle je oprava v klientovi, nebo měníte i něco na API?

@tomasfejfar
Copy link
Copy Markdown
Contributor

V API je nedokumentovaná "feature" (spíš bug), že to tahá ty hodnoty postupně ze všech $_GET, $_POST, $_SERVER, $_ENV, etc. Takže se nepoznalo, že to ten klient posílá getem i když by to měl být post. Takže je to fix jen v klientovi. API zůstává jak bylo. A dokonce je pořád možné tam poslat místo PUTu get. Resp. poslat parametry PUTu jako GET params v URL. Myslím, že s tím souvisí KBC-272

@ErikZigo
Copy link
Copy Markdown
Contributor

Pokud je to jen fix v klientovi tak je to v pohodě.

@romanbracinik
Copy link
Copy Markdown
Contributor Author

Jojo v connection sa nemenilo nic, funguje to tam uz historicky tak, ze to mozes poslat jak v GET tak ako ako hodnoty v PUTe. Takze API sa chova uplne rovnako.

@romanbracinik romanbracinik force-pushed the roman-fix-update-table-in-client branch from d617f72 to 958cfd2 Compare April 22, 2020 10:06
@zajca
Copy link
Copy Markdown
Member

zajca commented Apr 22, 2020

Každopádně by se měla po tomto vydat verze 11, protože je to BC v klientu.

// kdyby bylo mergnuté #350 tak už řve.

@romanbracinik
Copy link
Copy Markdown
Contributor Author

Jo to som za vydat novu verziu. Dam to este raz na review mal som nejaky konflikt po rebase tak pre istotu.

Copy link
Copy Markdown
Contributor

@tomasfejfar tomasfejfar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Přijde mi to stejné. A první dataset testů mi prošel. Takže LGTM

@zajca
Copy link
Copy Markdown
Member

zajca commented Apr 22, 2020

Co ti přijde stejné?
změnila se signatura metody z public function updateTable($tableId, $options) na public function updateTable(TableUpdateOptions $options) to je BC. Otázka jestly by se s novou major verzí neměli stejným způsobem změnit i další signatury metod ať používájí Options objekty (neprocházel jsem teda kolik jich teď je).

@tomasfejfar
Copy link
Copy Markdown
Contributor

Jo, já už to chápu jak to myslíš - že je to zbytečný BC break! No, to máš asi pravdu.

Copy link
Copy Markdown
Contributor

@tomasfejfar tomasfejfar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Zbytečný BC break

@romanbracinik romanbracinik force-pushed the roman-fix-update-table-in-client branch from af815c5 to 92aa268 Compare April 23, 2020 07:48
@romanbracinik
Copy link
Copy Markdown
Contributor Author

Screenshot 2020-04-23 at 09 49 22

Tak este jeden pokus :) Upravil som len tie parametre

@tomasfejfar tomasfejfar self-requested a review April 23, 2020 10:52
Copy link
Copy Markdown
Contributor

@tomasfejfar tomasfejfar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Viděl jsem projít testy.
image

@romanbracinik romanbracinik merged commit 8406d02 into master Apr 23, 2020
@romanbracinik romanbracinik deleted the roman-fix-update-table-in-client branch April 23, 2020 12:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants